Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UNOFFICIAL category for the United States #1669

Merged
merged 6 commits into from
Mar 1, 2024

Conversation

PPsyrius
Copy link
Collaborator

@PPsyrius PPsyrius commented Jan 26, 2024

Proposed change

This PR implements additional holidays (marked as UNOFFICIAL category), which includes:

  • Groundhog Day (FEB, 2).
  • Valentine's Day (FEB, 14) from 1847 onwards.
  • St. Patrick's Day (MAR, 17).
  • Halloween (OCT, 31).
  • (US Presidential) Election Day (Tuesday next after the 1st Monday in NOV) from 1848 onwards.

Closes #1643 .

Type of change

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency update (version deprecation/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new python-holidays functionality in general)

Checklist

  • I've followed the contributing guidelines
  • I've run make pre-commit, it didn't generate any changes
  • I've run make test, all tests passed locally

@coveralls
Copy link

coveralls commented Jan 26, 2024

Pull Request Test Coverage Report for Build 8106192363

Details

  • 45 of 45 (100.0%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 99.993%

Totals Coverage Status
Change from base Build 8105363435: -0.007%
Covered Lines: 10592
Relevant Lines: 10592

💛 - Coveralls

@PPsyrius
Copy link
Collaborator Author

PPsyrius commented Jan 26, 2024

Note: due to the current United States implementation, _populate_subdiv_holidays(self) will likely throw additional holidays into all test_[category_name]_holiday. I did try changing the former to _populate_subdiv_public_holidays(self), but that doesn't work either as the current syntactic sugar interprets public as a SUBDIV name.

@KJhellico
Copy link
Collaborator

KJhellico commented Jan 26, 2024

While we don't have NON_PUBLIC subdiv holidays, we can just add

        if PUBLIC not in self.categories:
            return None

to the beginning of _populate_subdiv_holidays().

When such holidays appear, we should change it to

       if PUBLIC in self.categories:
            # MLK Day ...

@KJhellico
Copy link
Collaborator

Happy Groundhog Day! 🎉😉

@PPsyrius
Copy link
Collaborator Author

PPsyrius commented Feb 6, 2024

Happy Groundhog Day! 🎉😉

It turns out from some checks they do celebrate this in Hawaii, too, instead of just the Continental U.S. huh, even if they didn't have any groundhogs there 👀

Copy link
Collaborator

@KJhellico KJhellico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This category (NON PUBLIC) opens up a very wide field of activity for us... ;)

@arkid15r arkid15r changed the base branch from beta to dev February 19, 2024 18:47
@PPsyrius
Copy link
Collaborator Author

If there any additional changes required for this PR? 👀

@arkid15r
Copy link
Collaborator

If there any additional changes required for this PR? 👀

No, not really. I was just thinking about NON_PUBLIC vs UNOFFICIAL naming. To me non-public is closer to private than unofficial. I know I kind of approved it in the issue discussion earlier. @KJhellico what's your opinion?

@KJhellico
Copy link
Collaborator

I also like UNOFFICIAL better. :)

@PPsyrius PPsyrius force-pushed the US_non-public_observance branch from 6cb297e to 79b513b Compare March 1, 2024 03:35
@PPsyrius PPsyrius changed the title Add NON_PUBLIC category for the United States. Add UNOFFICIAL category for the United States. Mar 1, 2024
Copy link

sonarqubecloud bot commented Mar 1, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@PPsyrius
Copy link
Collaborator Author

PPsyrius commented Mar 1, 2024

I think my bad habits of git pull --rebase then git push -f for my own private repos might have accidentally been used here..., sorry about that one 💀

Anyway, I've switched all instances of NON_PUBLIC to UNOFFICIAL now. @KJhellico @arkid15r

@PPsyrius PPsyrius requested a review from KJhellico March 1, 2024 03:40
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arkid15r arkid15r added this pull request to the merge queue Mar 1, 2024
Merged via the queue into vacanza:dev with commit e5492f9 Mar 1, 2024
26 checks passed
@PPsyrius PPsyrius deleted the US_non-public_observance branch March 2, 2024 15:34
@KJhellico KJhellico mentioned this pull request Mar 4, 2024
@mborsetti
Copy link
Contributor

Apologies, thanks for all the work on this library and this looks neat, but is there any documentation on how to use it?

I don't seem to find instructions on how to request "categories" in the documentation, yet I presume that any changes would have to be thoroughly documented to the user before a PR is merged. Where should I look?

Also, what is the definition and criteria for UNOFFICIAL (this seems to be missing from the documentation as well)? Would National Fruitcake Toss Day be included in this library or not? If so, why?

@PPsyrius
Copy link
Collaborator Author

PPsyrius commented Mar 5, 2024

@mborsetti I'm pretty sure they're listed in https://python-holidays.readthedocs.io/en/latest/api.html#holidays.holiday_base.HolidayBase

So, for example, calling up the Californian-specific list for 2024 with UNOFFICIAL list included would be

holidays.country_holidays("US", subdiv="CA", years=2024, categories=("public","unofficial",))

We might want to update categories=None to categories=PUBLIC for default ones as listed in https://github.com/vacanza/python-holidays/blob/dev/holidays/holiday_base.py unless I misunderstood our own codebase @KJhellico @arkid15r


In terms of criteria, there are a bunch of similarities with the existing WORKDAY category - although those are officially observed by their respective governments, for which "Cinco de Mayo" would've fallen in this category due to its US Congress concurrent resolution status.

UNOFFICIAL, on the other hand, are popular days that are widely observed by citizens of a particular jurisdiction with no official observance - for which "Valentine's Day", "Groundhog Day", and "Halloween" perfectly fit.

Unless "National Fruitcakes Toss Day" is celebrated with some events being held for it, I don't think it belongs in this category except for perhaps Manitou Springs, Colorado if we ever get to add that subdivision in once #1713 is implemented.

@arkid15r
Copy link
Collaborator

arkid15r commented Mar 5, 2024

Hey Mike, long time no see!

It's great you're still keeping an eye on the PH. I appreciate your reminder regarding keeping the docs up to date (which is not always the case).

Let's look into your concerns in details:

I don't seem to find instructions on how to request "categories"

The usage of categories mentioned here and in the Holiday categories support section. All supported categories specified on per entity basis and listed in the Available Countries table (Supported Categories column).

What documentation is missing is description of each category (I'll create an issue to address that). This leads us to your next concern

what is the definition and criteria for UNOFFICIAL

As there is no standard for holiday categories I trust other fellow contributors/maintainer with their choice unless I clearly notice something is not fit. Normally we resolve this kind of concerns during code reviews or in issues.

My general observation is that PH could use a better implemented/documented code ratio. Neither of the current maintainers are native English speakers. But if you want to blame somebody for that -- it'd be me. I'm responsible for this level of documentation quality (normally I review and approve every PR merged into dev/main branch -- btw, we moved to the new branch naming recently!). You can see @PPsyrius's response above with his idea of UNOFFICIAL hoiiday meaning. I can't agree more that each category purpose should be properly documented.

I also think it's worth to mention that I've started planning PH v1 roadmap recently. It has a separate issue for better documentation. If you have any suggestions/ideas please contribute.

@arkid15r
Copy link
Collaborator

arkid15r commented Mar 5, 2024

We might want to update categories=None to categories=PUBLIC for default ones as listed in https://github.com/vacanza/python-holidays/blob/dev/holidays/holiday_base.py unless I misunderstood our own codebase @KJhellico @arkid15r

It's handled here. The default category is PUBLIC and designed to be modifiable for better flexibility.

@mborsetti
Copy link
Contributor

Hey Arkadii,

Thanks for your response, and great leadership!

I do get notices of new releases and when I have time check in on all the great improvements; wonderful job, I can't believe how many countries/subdivisions are supported now!

I also think it's worth to mention that I've started planning PH v1 roadmap recently.

Wow, wonderful to see; the API does indeed need rethinking IMHO. I see if I have time to do a deeper dive.

@arkid15r
Copy link
Collaborator

arkid15r commented Mar 5, 2024

I do get notices of new releases and when I have time check in on all the great improvements; wonderful job, I can't believe how many countries/subdivisions are supported now!

That's right! A lot of people contributed to PH's success. However, personally I'm really proud of the quality of the data provided by PH. @KJhellico and @PPsyrius have been doing amazing work on hunting down every single holiday in their contributions.

@arkid15r arkid15r changed the title Add UNOFFICIAL category for the United States. Add UNOFFICIAL category for the United States Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional Holidays
5 participants